sequenceDiagram
  participant Client
  participant ManagementAPI
  participant Transformer
  participant XdsTranslator
  participant UpstreamDefinitions
  participant Envoy

  Client->>ManagementAPI: deploy RestApi (may include per-op upstream.ref)
  ManagementAPI->>Transformer: StoredConfig -> RuntimeDeployConfig (include per-op upstream refs)
  Transformer->>UpstreamDefinitions: resolve per-op ref via upstreamref.FindByName
  Transformer->>XdsTranslator: emit routes referencing definition cluster keys
  XdsTranslator->>Envoy: create/register clusters (API-scoped clusterkey.APILevel / reused definition clusters)
  Envoy-->>Client: traffic routed to correct backend (per-op or API-level)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

All committers have signed the CLA.

@coderabbitai review

@coderabbitai review

@coderabbitai review

@coderabbitai review

@coderabbitai review

@coderabbitai review

@coderabbitai review

@coderabbitai I'd like your opinion on a design decision in the operation-level upstream schema.

Context (gateway-controller/api/management-openapi.yaml):

• API-level Upstream allows oneOf: [url, ref], so a backend can be given as a raw URL or as a ref to a named upstreamDefinitions entry.
• Operation-level RestAPIOperationUpstream has the same main and sandbox sub-fields as the API level, BUT each sub-field is a RestAPIOperationUpstreamTarget, which is required: [ref] (ref-only). A raw inline url is intentionally NOT allowed at the operation level.

Question: is restricting per-operation overrides to ref-only (no inline url) the right call, or should we mirror the API level and also allow url here?

Please weigh in on these specific angles by looking at the actual code:

1. Envoy/EDS cluster stability: does ref-only (one named definition = one reusable, stable cluster) actually avoid cluster churn vs. allowing an ad-hoc cluster per inline URL? Trace how the xDS translator builds clusters from upstream-definition refs.
2. Feature parity: upstreamDefinitions carry basePath, timeout, and weighted upstreams (load balancing). If we allowed a raw per-op url, would those settings be unreachable at the operation level?
3. Validation / single source of truth: is validating all backend URLs once in spec.upstreamDefinitions and referencing by name simpler and less error-prone than re-validating inline URLs on every operation?
4. Any downsides, edge cases, or inconsistencies you see in keeping operation-level ref-only while the API level stays url-or-ref?

Please confirm whether the ref-only restriction is sound, or flag concerns.

#!/bin/bash
# Trace cluster building in xDS translator for per-op refs vs URL-based upstreams
rg -n "resolvePerOpDefinitionCluster\|resolveUpstreamCluster\|sanitizeEnvoyClusterName\|UpstreamDefinitionClusterPrefix\|perOpDefinitionClusterKey" --type go -A 5 -B 2

echo "---CLUSTERKEY---"
cat gateway/gateway-controller/pkg/utils/clusterkey/clusterkey.go 2>/dev/null || fd -i "clusterkey.go" --exec cat {}

echo "---UPSTREAMREF---"
cat gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go 2>/dev/null || fd -i "upstreamref.go" --exec cat {}

echo "---XDS_TRANSLATOR_CLUSTER_BUILD---"
rg -n "createCluster\|ConnectTimeout\|UpstreamDefinition\|basePath\|ClusterKey\|EnvoyClusterName" gateway/gateway-controller/pkg/xds/translator.go -A 3 -B 2

echo "---OPERATION_UPSTREAM_SCHEMA---"
rg -n "RestAPIOperationUpstream\|RestAPIOperationUpstreamTarget\|ref-only\|inline" gateway/gateway-controller/api/management-openapi.yaml -A 8 -B 2

echo "---UPSTREAM_DEF_FIELDS---"
rg -n "basePath\|BasePath\|timeout\|Timeout\|weighted\|Weighted\|upstreams\|Upstreams" gateway/gateway-controller/pkg/api/management/generated.go -B 2 -A 2 | head -120

#!/bin/bash
# Find actual file paths
fd -i "translator.go" --type f | grep -v test
fd -i "management-openapi.yaml" --type f
fd -i "generated.go" --type f | grep management

echo "---SEARCH FOR KEY SYMBOLS---"
rg -rn "resolvePerOpDefinitionCluster\|UpstreamDefinitionClusterPrefix\|perOpDefinitionClusterKey" --type go -A 8 -B 2

echo "---UPSTREAM DEF STRUCT---"
rg -rn "type UpstreamDefinition\|BasePath\|basePath\|Weighted\|weighted" --type go -A 3 -B 1 | head -80

echo "---OPERATION UPSTREAM OPENAPI---"
rg -rn "RestAPIOperationUpstream\|RestAPIOperationUpstreamTarget\|ref-only" -A 8 -B 2 | head -120

echo "---CLUSTER BUILD IN TRANSLATOR---"
rg -rn "createCluster\|ConnectTimeout\|ClusterKey\|EnvoyClusterName\|UpstreamDefinition" --type go -A 4 -B 2 | grep -v "_test.go" | head -200

#!/bin/bash
echo "---TRANSLATOR---"
cat gateway/gateway-controller/pkg/xds/translator.go

echo "---OPENAPI YAML OPERATION UPSTREAM---"
grep -n "RestAPIOperation\|Operation\|upstream\|ref\|basePath\|timeout\|weighted" gateway/gateway-controller/api/management-openapi.yaml | head -120

echo "---UPSTREAM DEFINITION STRUCT IN GENERATED---"
grep -n "UpstreamDefinition\|BasePath\|Weighted\|Timeout\|Connect\|UpstreamTimeout" gateway/gateway-controller/pkg/api/management/generated.go | head -80

echo "---RESTAPI TRANSFORMER KEY SECTIONS---"
grep -n "perOp\|per_op\|PerOp\|DefinitionCluster\|definitionCluster\|UpstreamDef\|upstreamDef\|clusterkey\|upstreamref" gateway/gateway-controller/pkg/transform/restapi.go | head -80

@mehara-rothila — based on reviewing the actual translator and transformer code, the ref-only restriction is well-founded. Here is the analysis across each angle.

1. Envoy/EDS cluster stability

The cluster name for a per-op override is computed as:

// translator.go – resolvePerOpDefinitionCluster
clusterName := constants.UpstreamDefinitionClusterPrefix + kind + "_" + apiID + "_" + sanitizeUpstreamDefinitionName(definition.Name)

The name is derived from the definition name, never from the URL. The upstream-definitions loop creates this cluster unconditionally, so any number of operations sharing the same ref reuse a single pre-existing cluster — zero per-op cluster creation. Compare this with what would happen if inline URLs were allowed: either a new cluster would be minted per operation (O(N) cluster churn on every config push when N operations exist), or a stable name would need to be invented without a definition-name anchor, which would require an entirely separate naming and EDS-stability scheme. The current design avoids both problems.

2. Feature parity

UpstreamDefinition carries three capabilities that a raw inline URL cannot provide:

If a raw URL were permitted at the operation level, those users would silently get a stripped-down backend configuration. The ref-only constraint ensures all per-op overrides have full parity with what the definition system provides.

3. Validation / single source of truth

Both the xDS translator (resolveUpstreamDefinition → upstreamref.FindByName) and the RDC transformer (perOpDefinitionClusterKey → upstreamref.FindByName) resolve refs through the same utility function:

// upstreamref.go
func FindByName(ref string, defs *[]api.UpstreamDefinition) (*api.UpstreamDefinition, error) { ... }

URL validation (scheme, host, parse errors) is performed once per definition entry, not per operation. If inline URLs were allowed at the operation level, URL parsing and scheme validation would need to be duplicated in validateRestData/validateOperations, and the xDS translator would need a new code branch that bypasses resolvePerOpDefinitionCluster. The current design keeps the validator's operation upstream check as a pure ref-existence lookup, which is significantly simpler and less error-prone.

4. Downsides and edge cases

The one genuine downside is boilerplate for trivial cases: if a user wants to route a single operation to a different backend, they must declare an upstreamDefinition entry even if it is referenced only once. This is a real usability tax for simple scenarios.

Beyond that, the main structural consideration is the asymmetry between API-level (url or ref) and operation-level (ref-only). This asymmetry is intentional and defensible: the API-level url shorthand exists as a convenience for the majority of APIs where a single backend needs no named definition. A per-operation upstream override is, by definition, a more advanced routing scenario — the kind of scenario where the cluster reuse, basePath inheritance, and timeout control that definitions provide are most valuable. Offering a raw URL shortcut at the operation level would add implementation complexity (new cluster-naming scheme, new validation path, duplicate logic) while delivering a worse-featured result for users.

Summary: The ref-only restriction is sound. It preserves EDS-stable cluster identity, delivers full feature parity (basePath, timeout, weighted load balancing), and eliminates a duplicate validation and cluster-naming code path. The only trade-off — mandatory definition declaration even for single-use refs — is reasonable given the benefits. If you find that single-use boilerplate becomes a friction point in practice, it could be addressed later by adding a convenience url shorthand that internally auto-generates a definition, but that would be a separate, additive change and is not needed now.

@coderabbitai review the per-operation upstream schema (RestAPIOperationUpstream + RestAPIOperationUpstreamTarget) in gateway/gateway-controller/api/management-openapi.yaml. Is it well-formed and consistent with how the Go validator and transform/xds consume it? Flag any changes, or confirm it's fine. Give your own independent read.

#!/bin/bash
echo "===OPENAPI YAML - FULL OPERATION UPSTREAM SCHEMAS==="
grep -n "RestAPIOperationUpstream\|RestAPIOperationUpstreamTarget\|Operation:\|operations:" gateway/gateway-controller/api/management-openapi.yaml | head -60

echo ""
echo "===FULL SCHEMA BLOCKS==="
python3 - <<'EOF'
import re

with open("gateway/gateway-controller/api/management-openapi.yaml") as f:
    content = f.read()

# Extract RestAPIOperationUpstream and RestAPIOperationUpstreamTarget blocks
for name in ["RestAPIOperationUpstream:", "RestAPIOperationUpstreamTarget:", "Operation:"]:
    idx = content.find(f"    {name}")
    if idx == -1:
        idx = content.find(name)
    if idx != -1:
        print(f"\n===SCHEMA: {name}===")
        print(content[idx:idx+600])
        print("...")
EOF

echo ""
echo "===GENERATED.GO - RestAPIOperationUpstream types==="
grep -n "RestAPIOperationUpstream\|RestAPIOperationUpstreamTarget\|union json\|MarshalJSON\|UnmarshalJSON\|AsRestAPI\|FromRestAPI\|MergeRestAPI" gateway/gateway-controller/pkg/api/management/generated.go | head -80

echo ""
echo "===GENERATED.GO - struct bodies==="
python3 - <<'EOF'
with open("gateway/gateway-controller/pkg/api/management/generated.go") as f:
    content = f.read()

for name in ["RestAPIOperationUpstream", "RestAPIOperationUpstreamTarget", "RestAPIOperationUpstream0", "RestAPIOperationUpstream1"]:
    idx = content.find(f"type {name} struct")
    if idx == -1:
        idx = content.find(f"type {name} =")
    if idx != -1:
        print(f"\n===TYPE: {name}===")
        print(content[idx:idx+300])
EOF

echo ""
echo "===VALIDATOR - operation upstream validation==="
grep -n "op\.Upstream\|Upstream\.Main\|Upstream\.Sandbox\|upstreamRef\|OperationUpstream\|RestAPIOperationUpstream" gateway/gateway-controller/pkg/config/api_validator.go -A 5 -B 2 | head -160

echo ""
echo "===TRANSFORMER - per-op upstream consumption==="
grep -n "op\.Upstream\|Upstream\.Main\|Upstream\.Sandbox\|PerOp\|perOp\|perOpDefinitionClusterKey" gateway/gateway-controller/pkg/transform/restapi.go -A 5 -B 2 | head -160

echo ""
echo "===XDS TRANSLATOR - per-op upstream consumption==="
grep -n "op\.Upstream\|Upstream\.Main\|Upstream\.Sandbox\|resolvePerOpDefinitionCluster\|PerOp\|perOp" gateway/gateway-controller/pkg/xds/translator.go -A 5 -B 2 | head -160

@mehara-rothila — here is the independent read.

Overall verdict: mostly well-formed and consistent, with one structural concern worth addressing.

What's correct and consistent

Concern: anyOf generates problematic union boilerplate

The anyOf clause triggers oapi-codegen's union-type machinery. This produces:

// Generated dead code — both are just interface{}
type RestAPIOperationUpstream0 = interface{}
type RestAPIOperationUpstream1 = interface{}

// Plus As/From/Merge methods for each — also dead code

type RestAPIOperationUpstream struct {
    Main    *RestAPIOperationUpstreamTarget `json:"main,omitempty"`
    Sandbox *RestAPIOperationUpstreamTarget `json:"sandbox,omitempty"`
    union   json.RawMessage                 // ← populated only by UnmarshalJSON
}

func (t RestAPIOperationUpstream) MarshalJSON() ([]byte, error) {
    b, err := t.union.MarshalJSON()  // ← returns null if union is nil
    ...
}

The latent bug: MarshalJSON delegates entirely to the union json.RawMessage field. That field is populated only during UnmarshalJSON (i.e., when a client sends JSON to the API). Any code path that constructs a RestAPIOperationUpstream directly in Go — such as a GET response that loads config from the database and re-serializes it — will have union == nil and will serialize as null instead of {"main":{"ref":"foo"}}.

The transformer and xDS translator access op.Upstream.Main and op.Upstream.Sandbox directly and never call MarshalJSON, so those paths are unaffected. But the API response path (GET endpoint) will hit this if the stored config struct is serialized back to JSON.

The anyOf constraint itself is also redundant: the Go validator already enforces "at least one of main/sandbox" independently. The schema-level constraint does not add server-side protection beyond what the validator provides.

Recommended fix: replace the anyOf with minProperties: 1 if oapi-codegen supports it without generating union code, or simply remove the anyOf and rely on the validator + the existing description text for the constraint. Either approach eliminates the union boilerplate and the MarshalJSON null risk.

# Current — generates union machinery
RestAPIOperationUpstream:
  type: object
  additionalProperties: false
  anyOf:
    - required: [main]
    - required: [sandbox]
  properties: ...

# Suggested — no union generation; constraint documented and enforced by validator
RestAPIOperationUpstream:
  type: object
  additionalProperties: false
  minProperties: 1   # if generator respects this without union codegen
  description: "... At least one of main or sandbox must be set (enforced by server-side validation)."
  properties: ...

If minProperties: 1 also triggers union code in your generator version, removing the anyOf entirely is the safer option — the validator enforcement is the authoritative gate.

Summary

The schema accurately models the intended constraints, and all field-level rules (pattern, maxLength, minLength, additionalProperties) are faithfully mirrored in the validator. The one actionable item is the anyOf clause: it generates dead code and a MarshalJSON-null risk that could surface in the API GET response path. Everything the transformer and xDS translator consume is the struct fields directly, so those paths are sound regardless.

@coderabbitai Investigate the policy-building flow in the wso2/api-platform gateway-controller (Go) on the feat/per-op-upstream-gateway branch. I want an independent answer to whether DerivePolicyFromAPIConfig is actually used, and what the real policy path is.

Context , policy building appears to exist in two places:

1. pkg/policy/builder.go , DerivePolicyFromAPIConfig(...) returns *models.StoredPolicyConfig (held in pkg/storage/policy_store.go). Its doc comment claims it is used by APIDeploymentService, APIServer handlers, and main.go startup, but it also carries a TODO: "APIServer handlers (REST API path) - TODO: Refactor this to use the implementation."
2. pkg/transform/restapi.go , RestAPITransformer.Transform builds policy chains via buildPolicyChain(...) into rdc.PolicyChains, which pkg/policyxds consumes and pushes to the policy engine.

A grep across the whole gateway directory found NO callers of DerivePolicyFromAPIConfig (only its own definition in builder.go).

Please verify independently and report, with file:line evidence:

1. Is DerivePolicyFromAPIConfig (builder.go) called anywhere in the codebase? If yes, where exactly? If not, is it dead/orphaned code? Also check whether anything ever writes to policy_store.
2. What is the ACTUAL active path that builds per-route policy chains and feeds the policy engine for a deployed REST API (trace it from the deploy event to the policy engine)? Is it the restapi.go transform -> rdc.PolicyChains -> policyxds path, or builder.go, or both?
3. The per-op feature modified the sandbox vhost-gating in builder.go (added perOpSandboxHasContent). Is that copy on the active path, or did it edit an unused function? Is the equivalent gating in restapi.go (and xds/translator.go) the one actually exercised at runtime?
4. Bottom line: did the per-op work correctly wire per-op (incl. per-op sandbox) policy building through the active path, or is there a real gap (e.g., builder.go was supposed to be the path but is never called)? Are we missing anything, or is building it only in the transform path correct?

Give your own independent verdict.

#!/bin/bash
echo "===1. CALLERS OF DerivePolicyFromAPIConfig==="
rg -rn "DerivePolicyFromAPIConfig" --type go

echo ""
echo "===2. POLICY_STORE WRITES==="
rg -rn "policy_store\|PolicyStore\|StoredPolicyConfig\|SetPolicy\|SavePolicy\|WritePolicy\|StorePolicy" --type go | grep -v "_test.go" | head -80

echo ""
echo "===3. FULL BUILDER.GO==="
cat gateway/gateway-controller/pkg/policy/builder.go

echo ""
echo "===4. FULL POLICY_STORE.GO==="
fd -i "policy_store.go" --exec cat {}

echo ""
echo "===5. DEPLOY EVENT → TRANSFORM CHAIN==="
rg -rn "RestAPITransformer\|Transform\(\|Transform\b\|\.Transform" --type go | grep -v "_test.go" | head -80

echo ""
echo "===6. POLICYXDS CONSUMPTION==="
fd -i "policyxds" --type d
rg -rn "PolicyChains\|policyxds\|PushPolicy\|BuildPolicy" --type go | grep -v "_test.go" | head -80

echo ""
echo "===7. MAIN.GO OR DEPLOYMENT SERVICE WIRING==="
fd -i "main.go" --type f | xargs grep -l "policy\|Policy\|transformer\|Transformer" 2>/dev/null
rg -rn "APIDeploymentService\|deploymentService\|DeployAPI\|HandleDeploy\|RestAPITransformer\|TransformerRegistry\|transformerRegistry" --type go | grep -v "_test.go" | head -80

echo ""
echo "===8. HOW SANDBOX GATING APPEARS IN BOTH BUILDER AND RESTAPI==="
grep -n "perOpSandboxHasContent\|apiSandboxHasContent\|hasSandbox\|SandboxHas" gateway/gateway-controller/pkg/policy/builder.go gateway/gateway-controller/pkg/transform/restapi.go 2>/dev/null

#!/bin/bash
echo "===1. CALLERS OF 'n' FUNCTION (non-test) FROM POLICY PACKAGE==="
rg -rn "\bpolicybuilder\.n\b\|\bpolicy\.n\b\|DerivePolicyFromAPIConfig" --type go | grep -v "_test.go"

echo ""
echo "===2. CONTROLLER MAIN.GO FULL==="
cat gateway/gateway-controller/cmd/controller/main.go

echo ""
echo "===3. POLICYXDS PACKAGE CONTENTS==="
fd --type f --extension go "." gateway/gateway-controller/pkg/policyxds/ | sort | xargs -I{} sh -c 'echo "---FILE: {}---" && cat {}'

echo ""
echo "===4. RESTAPITRANSFORMER - WHERE IS IT INSTANTIATED AND CALLED==="
rg -rn "RestAPITransformer\|NewRestAPITransformer\|transformerRegistry\|TransformerRegistry\|\.Transform(" --type go | grep -v "_test.go"

echo ""
echo "===5. POLICYSTORE.SET CALLERS (production)==="
rg -rn "\.Set(" --type go | grep -i "policy\|store" | grep -v "_test.go"

echo ""
echo "===6. POLICYXDS MAIN ENTRYPOINT AND HOW IT GETS RDC==="
fd --type f --extension go "." gateway/gateway-controller/pkg/policyxds/ | head -5

echo ""
echo "===7. HOW STOREDPOLICYCONFIG IS PRODUCED AND CONSUMED==="
rg -rn "StoredPolicyConfig\|StoredPolicyConf" --type go | grep -v "_test.go" | head -40

echo ""
echo "===8. WHAT PRODUCES rdc.PolicyChains AND WHO CONSUMES==="
rg -rn "PolicyChains\|\.PolicyChains" --type go | grep -v "_test.go" | head -40

echo ""
echo "===9. APISERVER HANDLER PRODUCTION CODE==="
fd --type f --extension go "." gateway/gateway-controller/pkg/api/handlers/ | grep -v test | sort | head -20
rg -rn "DeployAPI\|HandleDeploy\|apiDeployment\|APIDeployment\|DeploymentService\|deploymentService\|policybuilder\|DerivePolicyFrom" --type go | grep -v "_test.go" | head -50

@mehara-rothila — here is the independent verdict with file:line evidence from the actual code.

1. Is DerivePolicyFromAPIConfig called anywhere in production?

No. It is dead production code.

The function exported in the doc comment as DerivePolicyFromAPIConfig is actually named n (unexported, single-letter) in pkg/policy/builder.go:

// DerivePolicyFromAPIConfig derives a policy configuration from an API stored config.
// Handles both RestApi and WebSubApi kinds. This is a shared utility used by:
// - APIDeploymentService (WebSocket event path)
// - APIServer handlers (REST API path) - TODO: Refactor this to use the implementation
// - main.go startup (loading existing configs)
func n(cfg *models.StoredConfig, ...) *models.StoredPolicyConfig {

A search for every caller of n across the entire codebase returns only test files:

Zero matches in any non-test file.

PolicyStore (pkg/storage/policy_store.go) also confirms this: it has a Set method, but no production code calls it. The doc comment claiming the function is used by "APIDeploymentService", "APIServer handlers", and "main.go startup" is factually incorrect and will mislead future maintainers.

2. What is the actual active policy-building path?

The active path, traced from cmd/controller/main.go, is:

main.go
  → transform.NewRestAPITransformer(...)             // line ~307
  → transform.NewRegistry(restTransformer, ...)      // line ~310
  → policyManager.SetTransformers(transformerRegistry)  // line ~312
  → loadRuntimeConfigsFromExistingAPIConfigurations(...)  // startup hydration
      → transformerRegistry.Transform(cfg)           // per each StoredConfig
  → [on deploy] policyManager.UpsertAPIConfig(cfg)   // pkg/policyxds/manager.go
      → pm.transformers.Transform(cfg)               // calls RestAPITransformer.Transform
          → buildPolicyChain(...)                    // pkg/transform/restapi.go
          → rdc.PolicyChains populated
      → runtimeStore.Set(key, rdc)
      → policySnapshotManager.UpdateSnapshot(ctx)
          → translator.TranslateRuntimeConfigs(rdcs)
          → policyCache.SetResources(...)            // pushed to policy engine via xDS

RestAPITransformer.Transform in pkg/transform/restapi.go is the sole production builder for rdc.PolicyChains. builder.go / n is never on this path.

3. Is the builder.go sandbox-gating change on the active path?

No. The perOpSandboxHasContent predicate added in builder.go (Line 185) modifies the function n, which has zero production callers. That change is on dead code.

The sandbox gating that is actually exercised at runtime lives in pkg/transform/restapi.go:

// restapi.go – active path
apiSandboxHasContent := ...  // Line 145
hasSandbox := apiSandboxHasContent
// per-op override loop sets hasSandbox = true when op.Upstream.Sandbox != nil  // Line 151-156
if apiSandboxHasContent || (op.Upstream != nil && op.Upstream.Sandbox != nil) {  // Line 229
    // append sandbox vhost route
}

This logic is called through policyManager.UpsertAPIConfig on every deploy. The xds/translator.go path for per-op sandbox is also on the active Envoy xDS path and is correctly gated.

4. Bottom line: is the per-op work correctly wired?

Yes for the active path; no for builder.go.

There is no functional gap at runtime. The per-op sandbox override routes are built correctly through restapi.go and pushed to the policy engine via policyxds.

The actionable issue is builder.go itself. It carries a misleading doc comment claiming production usage, is tested as if it were a live component, and accumulates changes (including the perOpSandboxHasContent addition in this PR) that have no runtime effect. This creates ongoing maintenance risk: future per-op features added only to builder.go will silently not deploy, and the test coverage against it creates false confidence. The function should either be removed or the doc comment corrected to mark it explicitly as test-only.

Would you like to open a follow-up issue to track removal or correction of pkg/policy/builder.go (n) and pkg/storage/policy_store.go?

@coderabbitai review

I've applied a round of fixes from an external review on this per-operation upstream PR. Please give your own independent assessment of whether the current state is correct. Verify each of these against the actual code and flag anything wrong, inconsistent, or newly broken, with file:line:

• Connect-timeout validation: the regex is now ^\d+(\.\d+)?(ms|s|m|h)$ and the rejection message describes the single-unit, unsigned constraint. Is the validation correct and does it match the OpenAPI pattern?
• Per-op definition cluster naming: extracted into clusterkey.DefinitionName, used at all controller sites, with the two old sanitizers removed. Does it produce the exact same cluster name as before (no 503 / NoRoute risk)?
• Removed field: the unused UpstreamCluster.ConnectTimeout field and its reader were removed. Confirm nothing still depends on them.
• Shared schema: the UpstreamReference schema used by UpstreamDefinition.name, Upstream.ref, and the per-op target ref.

Don't assume the changes are right; give a skeptical, independent read.

#!/bin/bash
echo "===1. CONNECT-TIMEOUT VALIDATION==="
echo "---validator.go timeout regex and error messages---"
rg -n "timeout\|connect\|regex\|Regexp\|ParseDuration\|ParseConnect\|^\\\d\|ms|s|m|h\|unit\|unsigned\|single" gateway/gateway-controller/pkg/config/api_validator.go | head -60

echo ""
echo "---upstreamref.go ParseConnectTimeout---"
fd -i "upstreamref.go" --type f | grep -v test | xargs cat

echo ""
echo "---OpenAPI yaml pattern for timeout---"
rg -n "connect\|timeout\|pattern\|format\|Timeout" gateway/gateway-controller/api/management-openapi.yaml | head -40

echo ""
echo "===2. clusterkey.DefinitionName==="
echo "---clusterkey package full contents---"
fd -i "clusterkey.go" --type f | grep -v test | xargs cat

echo ""
echo "---old sanitizer presence check---"
rg -rn "sanitizeEnvoyClusterName\|sanitizeUpstreamDefinitionName\|sanitizeClusterName" --type go | grep -v "_test.go"

echo ""
echo "---DefinitionName callers across codebase---"
rg -rn "DefinitionName\|clusterkey\.DefinitionName" --type go | grep -v "_test.go"

echo ""
echo "---old cluster naming in translator.go---"
rg -n "UpstreamDefinitionClusterPrefix\|upstream_\|kind.*apiID\|apiID.*kind\|definition.*Name\|Name.*definition\|sanitize" gateway/gateway-controller/pkg/xds/translator.go | head -30

echo ""
echo "---old cluster naming in restapi.go---"
rg -n "UpstreamDefinitionClusterPrefix\|upstream_\|perOpDefinitionClusterKey\|DefinitionName\|sanitize" gateway/gateway-controller/pkg/transform/restapi.go | head -30

echo ""
echo "===3. UpstreamCluster.ConnectTimeout removed==="
echo "---models runtime_deploy_config.go---"
fd -i "runtime_deploy_config.go" --type f | grep -v test | xargs cat

echo ""
echo "---any remaining ConnectTimeout references in non-test go files---"
rg -rn "ConnectTimeout\|connect_timeout" --type go | grep -v "_test.go"

echo ""
echo "===4. Shared UpstreamReference schema==="
echo "---OpenAPI yaml UpstreamReference schema---"
python3 - <<'EOF'
import re
try:
    with open("gateway/gateway-controller/api/management-openapi.yaml") as f:
        content = f.read()
    # Find UpstreamReference schema
    idx = content.find("UpstreamReference:")
    if idx != -1:
        print(content[idx:idx+400])
    else:
        print("UpstreamReference not found in yaml")
    # Find all usages of UpstreamReference
    for m in re.finditer(r'UpstreamReference', content):
        start = max(0, m.start()-100)
        end = min(len(content), m.end()+200)
        print(f"\n---USAGE at char {m.start()}---")
        print(content[start:end])
except FileNotFoundError:
    print("File not found")
EOF

echo ""
echo "---rg for UpstreamReference in yaml---"
rg -n "UpstreamReference" gateway/gateway-controller/api/management-openapi.yaml -A 5 -B 5

echo ""
echo "---validator usage of upstreamRefRegex and name validation---"
rg -n "upstreamRefRegex\|UpstreamReference\|nameRegex\|NameRegex\|RefPattern\|refPattern" --type go | grep -v "_test.go" | head -40

#!/bin/bash
echo "===FULL VALIDATOR FILE - timeout and per-op sections==="
grep -n "timeout\|Timeout\|connect\|Connect\|ParseDuration\|regex\|Regex\|pattern\|Pattern\|upstreamRef\|UpstreamRef\|upstreamDefinitions\|UpstreamDefinition\|ref\|Ref" gateway/gateway-controller/pkg/config/api_validator.go | head -100

echo ""
echo "===FULL CLUSTERKEY FILE==="
cat gateway/gateway-controller/pkg/utils/clusterkey/clusterkey.go

echo ""
echo "===FULL RESTAPI.GO - cluster key usage==="
grep -n "clusterkey\|DefinitionName\|UpstreamDefinitionClusterPrefix\|constants\." gateway/gateway-controller/pkg/transform/restapi.go | head -40

echo ""
echo "===FULL TRANSLATOR.GO - cluster key usage==="
grep -n "clusterkey\|DefinitionName\|UpstreamDefinitionClusterPrefix\|constants\." gateway/gateway-controller/pkg/xds/translator.go | head -40

echo ""
echo "===CONSTANTS FILE==="
fd -i "constants.go" --type f | grep -i "gateway-controller" | xargs cat 2>/dev/null | head -60

echo ""
echo "===FULL RUNTIME DEPLOY CONFIG==="
cat gateway/gateway-controller/pkg/models/runtime_deploy_config.go

echo ""
echo "===POLICYXDS - ConnectTimeout usage==="
rg -rn "ConnectTimeout\|connect_timeout\|connectTimeout" --type go | grep -v "_test.go"

echo ""
echo "===OPENAPI YAML - full upstream and operation sections (500 lines around key areas)==="
python3 - <<'EOF'
try:
    with open("gateway/gateway-controller/api/management-openapi.yaml") as f:
        lines = f.readlines()
    for i, line in enumerate(lines):
        if any(kw in line for kw in ["UpstreamReference", "UpstreamDefinition", "RestAPIOperationUpstream", "Upstream:", "timeout", "connect", "pattern:", "UpstreamTimeout"]):
            start = max(0, i-2)
            end = min(len(lines), i+10)
            print(f"---Line {i+1}---")
            print("".join(lines[start:end]))
except FileNotFoundError:
    print("File not found")
EOF

@mehara-rothila — independent read complete. Here is the assessment for each item.

1. Connect-timeout validation

Verdict: Correct and consistent with the OpenAPI pattern.

api_validator.go Line 55:

connectTimeoutRegex: regexp.MustCompile(`^\d+(\.\d+)?(ms|s|m|h)$`),

management-openapi.yaml Line 4261:

pattern: '^\d+(\.\d+)?(ms|s|m|h)$'

These are identical.

The validation order (Lines 380–397) is correct:

1. time.ParseDuration fails → format error with the first message (covers abc, gibberish).
2. ParseDuration succeeds but regex fails → second message naming the unit constraint (covers -5s, +5s, 5ns, 100us, 1h30m).
3. Regex passes but d <= 0 → positivity error (covers 0s, 0.5ms is positive so passes, 0.0s is zero so caught).

One observation to be aware of: upstreamref.ParseConnectTimeout (upstreamref.go) does not apply the unit-contract regex — it uses only time.ParseDuration plus the positivity check. This means values like 5ns would pass ParseConnectTimeout at runtime if they somehow bypassed the validator. The validator is the correct enforcement point for this constraint, so this is not a bug in the current flow, but the asymmetry between the two parsers is worth a comment in ParseConnectTimeout to make the intent clear to future maintainers.

2. clusterkey.DefinitionName — cluster name equivalence

Verdict: Correct. Old sanitizers are fully removed; new function produces an equivalent name.

clusterkey.go:

func DefinitionName(kind, apiID, defName string) string {
    return constants.UpstreamDefinitionClusterPrefix + kind + "_" + apiID + "_" + sanitizeDefName(defName)
}
func sanitizeDefName(name string) string {
    name = strings.ReplaceAll(name, ".", "_")
    name = strings.ReplaceAll(name, ":", "_")
    return name
}

The old inline expression was UpstreamDefinitionClusterPrefix + kind + "_" + apiID + "_" + sanitizeUpstreamDefinitionName(definition.Name). The AI summary confirms the old sanitizer replaced dots and colons. The new sanitizeDefName does the same, so the output is identical.

Old sanitizer names (sanitizeEnvoyClusterName, sanitizeUpstreamDefinitionName, sanitizeClusterName) produce zero matches in production Go files — confirmed removed.

All four call sites (restapi.go Lines 266 and 452; translator.go Lines 946 and 1084) use the same argument order (kind, apiID, defName). No cluster-name divergence between paths. No 503/NoRoute risk.

One thing to note: the validator already rejects definition names with dots or colons via ^[a-zA-Z0-9\-_]+$, so for any newly created config the sanitization is a no-op. The replacement logic exists defensively for configs stored before the validator was tightened.

3. UpstreamCluster.ConnectTimeout removed

Verdict: Clean removal. Nothing depends on it.

runtime_deploy_config.go — UpstreamCluster struct confirmed:

type UpstreamCluster struct {
    Name      string
    BasePath  string
    Endpoints []Endpoint
    TLS       *UpstreamTLS
}

No ConnectTimeout field present. A search for ConnectTimeout and connect_timeout in all non-test Go files returns no matches. The field is fully gone with no remaining dependents.

Connect timeout still reaches Envoy: it flows as an argument directly into createCluster in translator.go at xDS translation time, bypassing the RDC model entirely. RouteTimeout.Connect on the Route struct is a separate, unrelated concept (per-route timeout) and is not affected.

4. Shared UpstreamReference schema

Verdict: Well-formed and consistently applied.

UpstreamReference (yaml Line 4210):

type: string
minLength: 1
maxLength: 100
pattern: '^[a-zA-Z0-9\-_]+$'

Used in three places:

The validator's upstreamRefRegex (^[a-zA-Z0-9\-_]+$) is an exact match with the OpenAPI pattern. The max-100/non-empty checks in the validator align with maxLength: 100 / minLength: 1 in the schema.

Using UpstreamReference for both definition name and for ref values creates a useful structural invariant: any definition name that passes validation is guaranteed to be directly referenceable from a per-op upstream target without a separate character-set conversion. This is sound.

Previously noted concern still open

The anyOf union boilerplate in generated.go (RestAPIOperationUpstream0/1, MarshalJSON delegating to union json.RawMessage) was raised in the prior review. That was not part of this fix set — the anyOf is still in the schema (yaml Line 4327) and the union code is still generated. The MarshalJSON-null risk on the GET response path remains.